Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Include account.owner into account hash #9917

Merged
merged 1 commit into from
May 7, 2020

Conversation

ryoqun
Copy link
Contributor

@ryoqun ryoqun commented May 7, 2020

Problem

account.owner isn't included into bank hash, and ultimately consensus.

Summary of Changes

hash it.

Fixes #9916

Part of #7167

@@ -3469,7 +3473,7 @@ pub mod tests {
};
let account = stored_account.clone_account();
let expected_account_hash =
Hash::from_str("GGTsxvxwnMsNfN6yYbBVQaRgvbVLfxeWnGXNyB8iXDyE").unwrap();
Copy link
Contributor Author

@ryoqun ryoqun May 7, 2020

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This time, I've checked against coreutils's sha256sum.

$ echo GGTsxvxwnMsNfN6yYbBVQaRgvbVLfxeWnGXNyB8iXDyE | base58 -d | od --format=x1
0000000 e2 d5 68 6a 18 6d 2d 29 84 f3 f3 ac b9 41 6c d4
0000020 7a 84 a1 17 69 83 18 ce b5 1f 1c 7e 90 2f c9 8d
0000040
$ (
  # lamports
  echo -ne '\x68\x67\x66\x65\x64\x63\x62\x61' &&
  # slot
  echo -ne '\xa0\x9f\x9e\x9d\x9c\x9b\x9a\x99' &&
  # rent epoch
  echo -ne '\x60\x5f\x5e\x5d\x5c\x5b\x5a\x59' &&
  # data
  echo -ne '\x28\x27\x26' &&
  # executable
  echo -ne '\x00' &&
  # pubkey
  echo -ne '\x88\x87\x86\x85\x84\x83\x82\x81\x80\x7f\x7e\x7d\x7c\x7b\x7a\x79\x78\x77\x76\x75\x74\x73\x72\x71\x70\x6f\x6e\x6d\x6c\x6b\x6a\x69'
) | sha256sum 
e2d5686a186d2d2984f3f3acb9416cd47a84a117698318ceb51f1c7e902fc98d  -

@@ -3469,7 +3473,7 @@ pub mod tests {
};
let account = stored_account.clone_account();
let expected_account_hash =
Hash::from_str("GGTsxvxwnMsNfN6yYbBVQaRgvbVLfxeWnGXNyB8iXDyE").unwrap();
Hash::from_str("5iRNZVcAnq9JLYjSF2ibFhGEeq48r9Eq9HXxwm3BxywN").unwrap();
Copy link
Contributor Author

@ryoqun ryoqun May 7, 2020

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This time, I've checked against coreutils's sha256sum.

$ echo 5iRNZVcAnq9JLYjSF2ibFhGEeq48r9Eq9HXxwm3BxywN | base58 -d | od --format=x1
0000000 46 0b 1e 3a 60 89 c1 c2 c5 81 9c 0f 80 47 0a 61
0000020 fe a8 92 17 00 82 9c 17 68 c4 52 76 9a 56 65 a9
0000040
$ (
  # lamports
  echo -ne '\x68\x67\x66\x65\x64\x63\x62\x61' &&
  # slot
  echo -ne '\xa0\x9f\x9e\x9d\x9c\x9b\x9a\x99' &&
  # rent epoch
  echo -ne '\x60\x5f\x5e\x5d\x5c\x5b\x5a\x59' &&
  # data
  echo -ne '\x28\x27\x26' &&
  # executable
  echo -ne '\x00' &&
  # owner
  echo -ne '\x58\x57\x56\x55\x54\x53\x52\x51\x50\x4f\x4e\x4d\x4c\x4b\x4a\x49\x48\x47\x46\x45\x44\x43\x42\x41\x40\x3f\x3e\x3d\x3c\x3b\x3a\x39' &&
  # pubkey
  echo -ne '\x88\x87\x86\x85\x84\x83\x82\x81\x80\x7f\x7e\x7d\x7c\x7b\x7a\x79\x78\x77\x76\x75\x74\x73\x72\x71\x70\x6f\x6e\x6d\x6c\x6b\x6a\x69'
) | sha256sum 
460b1e3a6089c1c2c5819c0f80470a61fea8921700829c1768c452769a5665a9  -

@codecov
Copy link

codecov bot commented May 7, 2020

Codecov Report

Merging #9917 into master will decrease coverage by 0.0%.
The diff coverage is 100.0%.

@@           Coverage Diff            @@
##           master   #9917     +/-   ##
========================================
- Coverage    80.4%   80.4%   -0.1%     
========================================
  Files         283     283             
  Lines       65868   65871      +3     
========================================
- Hits        52995   52981     -14     
- Misses      12873   12890     +17     

@mvines mvines added the automerge Merge this Pull Request automatically once CI passes label May 7, 2020
@solana-grimes solana-grimes merged commit 2ac5017 into solana-labs:master May 7, 2020
@ryoqun
Copy link
Contributor Author

ryoqun commented May 18, 2020

status update (tds): This has been successfully enabled on tds since slot 14_000_000.

@ryoqun
Copy link
Contributor Author

ryoqun commented May 23, 2020

status update (mainnet-beta): This has been successfully enabled on mainnet-beta since slot 12_500_000. As we learnt from devnet/tds, this is rolling-fashion and partially application, though

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
automerge Merge this Pull Request automatically once CI passes
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Account owner is not included in account hash
3 participants